Fix/public internal header #12374

azai91 · 2018-08-27T19:57:29Z

Description

the public header resource.h current has internal dependencies. public headers should only define interfaces and should not include internal dependencies as it allows users to access non-exposed interfaces. move random_generator.h into public header domain. move private implementations into respective .cc files.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

[ ]move random_generator.h into public header domain. move private implementations into respective .cc files.

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

lebeg · 2018-08-27T20:07:11Z

src/common/random_generator.cu

@@ -59,6 +59,16 @@ void RandGenerator<gpu, float>::Seed(mshadow::Stream<gpu> *s, uint32_t seed) {
  s->Wait();
 }

+
+static void RandGenerator<gpu, float>::AllocState(RandGenerator<gpu, DType> *inst) {


Isn't the only defined specialization double based?

lebeg · 2018-08-27T20:08:16Z

src/common/random_generator.h

-}  // namespace common
-}  // namespace mxnet
-#endif  // MXNET_COMMON_RANDOM_GENERATOR_H_
+///*


Is this on purpose?

need to update PR name, still working on this. commented out before deleting.

azai91 · 2018-08-27T21:38:00Z

@mseth10

lebeg · 2018-08-28T12:55:31Z

include/mxnet/random_generator.h

@@ -80,7 +79,7 @@ class RandGenerator<cpu, DType> {
    std::mt19937 *engine_;
  };

-  static void AllocState(RandGenerator<cpu, DType> *inst) {
+  static void  AllocState(RandGenerator<cpu, DType> *inst) {


some redundant whitespace

lupesko · 2018-08-30T16:24:18Z

public headers should only define interfaces and should include internal dependencies as it allows users to access non-exposed interfaces

@azai91 did you mean "...and should not include internal dependencies..." ?

@apeforest @samskalicky @anirudh2290 can you please take a look?

azai91 · 2018-08-30T16:25:27Z

you're right. will fix PR description.

samskalicky

LGTM

apeforest

@azai91 What issue/ticket does this change fix? Please link them in the PR. And why treat CPU and GPU implementation differently?

apeforest · 2018-08-30T16:26:45Z

include/mxnet/random_generator.h

-    CUDA_CALL(cudaMalloc(&inst->states_,
-                         kNumRandomStates * sizeof(curandStatePhilox4_32_10_t)));
-  }
+  static void AllocState(RandGenerator<gpu, DType> *inst);


Why do we do this differently with CPU implementation?

what do you mean? we are calling cudaMalloc in this case instead of just "new"

I guess my question is not well phrased. What I meant is why move out from header file while leaving the same function for CPU in the header

See line: https://github.com/apache/incubator-mxnet/pull/12374/files#diff-ba5bcd7d0b76b85a2df1f793dc4d3302R82

Aside from that, I think these functions are inside the inner class Impl which is supposed to handle all the implementation. Therefore I think it is very logical to leave them here in the header file. Not to mention the performance advantage of calling inline function.

I see what you mean. these cuda calls depends on cuda_utils.h which is in the commons folder. I am not sure if we want to expose those. thoughts?

@apeforest are you indicating about perf advantages during compile time ? I think its okay to put the definition in random_generator.cu since we don't want to expose cuda_utils.

@apeforest any thoughts on what to revise? if we want to be consistent we could create a random_generator.cc file and put the non-cuda implementations in there, but I personally think that is unnecessary for such small functions.

@azai91 Thanks for your efforts in refactoring this. Since the main purpose of this PR is refactoring, I hope we can do it in the most elegant way. If we are making this file a public header, I would extract the implementation class Impl to another internal header file so that we do not expose the internal implementation details. By doing that, we can have put the implementation details for both CPU and GPU there. Please let me know your thoughts.

it seems that they way we are using the random_generator, we expect the developer to be able to access the internal Impl class (https://github.com/azai91/incubator-mxnet/blob/6f7254c91709904a9fb6290f1998fcf2da818d0e/src/operator/random/unique_sample_op.h#L118).

the class is is public as well. I may be interpretting the developers intentions incorrectly though.

I suppose the developers did not intend to directly access internal Impl class but was rather lacking proper interface API. In fact, throughout the repository, I only found one place that uses RandGenerator<cpu, GType>::Impl` directly, and therefore I think it's not a big overhead to refactor that one line of code. Regarding how to separating Impl class from the interface, you might find this reference helpful: https://cpppatterns.com/patterns/pimpl.html

anirudh2290 · 2018-08-30T21:49:18Z

include/mxnet/random_generator.h

-    CUDA_CALL(cudaMalloc(&inst->states_,
-                         kNumRandomStates * sizeof(curandStatePhilox4_32_10_t)));
-  }
+  static void AllocState(RandGenerator<gpu, DType> *inst);


@apeforest are you indicating about perf advantages during compile time ? I think its okay to put the definition in random_generator.cu since we don't want to expose cuda_utils.

anirudh2290 · 2018-08-30T21:53:48Z

src/common/random_generator.cu

@@ -59,6 +59,17 @@ void RandGenerator<gpu, float>::Seed(mshadow::Stream<gpu> *s, uint32_t seed) {
  s->Wait();
 }

+template<>
+void RandGenerator<gpu, float>::AllocState(RandGenerator<gpu> *inst) {


doesnt this remove support for other DTypes like half_t ?

No, the random generator always generates floats between 0 and 1, we scale them as needed in wherever the random generator is used

apeforest

Approving this change given that this issue is blocking some customer development and a TODO ticket MXNET-948 has been created to refactor this class using a PIMP idiom.

aaronmarkham · 2018-09-14T23:39:56Z

I can't build mxnet on master from source anymore… getting this error:

make: *** No rule to make target 'include/mxnet/././../../src/common/random_generator.h', needed by 'build/src/operator/random/sample_multinomial_op.o'.  Stop.

I updated my submodules... I just pulled latest:

ubuntu@ip-172-31-66-78:~/incubator-mxnet$ git log
commit e213286db5f885701ee10d0a312c15564959edd9
Author: Sandeep Krishnamurthy <[email protected]>
Date:   Fri Sep 14 10:18:49 2018 -0700

Any ideas?

* move random-gen header to internal code * uncomment header * remove public method from cc * move cuda randgen logic to cc * remove private files * include correct header in cu random_gen * fix include in cu random-gen * fix template * remove static kw * add missing template expression * change dtype to float * remove old header * fix lint * fix imports * fix space * fix template * fix template * add todo * fix lint

azai91 requested a review from anirudh2290 as a code owner August 27, 2018 19:57

lebeg reviewed Aug 27, 2018

View reviewed changes

azai91 changed the title ~~Fix/public internal header~~ [incomplete] Fix/public internal header Aug 27, 2018

azai91 force-pushed the fix/public-internal-header branch 6 times, most recently from 263ad0a to b07262f Compare August 27, 2018 21:22

azai91 changed the title ~~[incomplete] Fix/public internal header~~ Fix/public internal header Aug 27, 2018

azai91 force-pushed the fix/public-internal-header branch 3 times, most recently from 65bc335 to ac51562 Compare August 27, 2018 23:51

lebeg reviewed Aug 28, 2018

View reviewed changes

azai91 force-pushed the fix/public-internal-header branch 3 times, most recently from 556f83d to 4c53b38 Compare August 28, 2018 15:19

samskalicky approved these changes Aug 30, 2018

View reviewed changes

apeforest suggested changes Aug 30, 2018

View reviewed changes

anirudh2290 reviewed Aug 30, 2018

View reviewed changes

lebeg approved these changes Aug 31, 2018

View reviewed changes

azai91 added 6 commits September 10, 2018 15:17

move random-gen header to internal code

dd06489

uncomment header

cb2cf5c

remove public method from cc

9662fe7

move cuda randgen logic to cc

6f92bf4

remove private files

9371d4c

include correct header in cu random_gen

eff6d60

azai91 force-pushed the fix/public-internal-header branch 23 times, most recently from 633d44e to 19fcae6 Compare September 12, 2018 01:53

add todo

2a714c3

apeforest approved these changes Sep 12, 2018

View reviewed changes

fix lint

e6fb503

anirudh2290 merged commit 7735fa6 into apache:master Sep 13, 2018

aaronmarkham mentioned this pull request Sep 17, 2018

sample_multinomial_op.o causing build from source to fail #12578

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/public internal header #12374

Fix/public internal header #12374

azai91 commented Aug 27, 2018 •

edited

Loading

lebeg Aug 27, 2018

lebeg Aug 27, 2018

azai91 Aug 27, 2018

azai91 commented Aug 27, 2018

lebeg Aug 28, 2018

lupesko commented Aug 30, 2018

azai91 commented Aug 30, 2018

samskalicky left a comment

apeforest left a comment

apeforest Aug 30, 2018

azai91 Aug 30, 2018 •

edited

Loading

apeforest Aug 30, 2018 •

edited

Loading

azai91 Aug 30, 2018

anirudh2290 Aug 30, 2018

azai91 Sep 10, 2018

apeforest Sep 10, 2018

azai91 Sep 11, 2018

apeforest Sep 12, 2018 •

edited

Loading

anirudh2290 Aug 30, 2018

anirudh2290 Aug 30, 2018

samskalicky Aug 31, 2018

apeforest left a comment •

edited

Loading

aaronmarkham commented Sep 14, 2018

Fix/public internal header #12374

Fix/public internal header #12374

Conversation

azai91 commented Aug 27, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azai91 commented Aug 27, 2018

Choose a reason for hiding this comment

lupesko commented Aug 30, 2018

azai91 commented Aug 30, 2018

samskalicky left a comment

Choose a reason for hiding this comment

apeforest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azai91 Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

apeforest Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apeforest Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apeforest left a comment • edited Loading

Choose a reason for hiding this comment

aaronmarkham commented Sep 14, 2018

azai91 commented Aug 27, 2018 •

edited

Loading

azai91 Aug 30, 2018 •

edited

Loading

apeforest Aug 30, 2018 •

edited

Loading

apeforest Sep 12, 2018 •

edited

Loading

apeforest left a comment •

edited

Loading